-
Notifications
You must be signed in to change notification settings - Fork 9.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
clientv3: version checking #7342
Conversation
So this only checks against clients <3.2 trying to connect to 3.2> with this flag on, right? |
func (c *Client) checkVersion() (err error) { | ||
var wg sync.WaitGroup | ||
errc := make(chan error, len(c.cfg.Endpoints)) | ||
ctx, cancel := context.WithCancel(c.ctx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does cancel
func preserver even when we overwrite ctx
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, canceling the parent context will cancel any descendents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see it now context.WithTimeout(ctx
, not context.WithTimeout(c.ctx
.
Makes sense.
clientv3/config.go
Outdated
@@ -41,6 +42,13 @@ type Config struct { | |||
// Password is a password for authentication. | |||
Password string `json:"password"` | |||
|
|||
// UseLatestCluster when set will refuse to create a client against an outdated cluster. | |||
UseLatestCluster bool `json:"use-latest-cluster"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EnsureClusterVersion? Or CheckClusterVersion? use
is not a strong enough word I feel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RejectOldCluster
? Should have something about whether it's new/old/latest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RejectOldCluster looks good.
clientv3/client.go
Outdated
min, rerr = strconv.Atoi(vs[1]) | ||
} | ||
if maj < 3 || min < 2 { | ||
rerr = rpctypes.ErrNotCapable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall we define a different error for cluster version mismatch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the fence about it. I feel it's similar enough to NotCapable
(i.e., a "soft" not capable) that the error can be reused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NotCapable is the error returned from server (at least for its current definition). The string is "etcdserver: not capable"
. It has the server prefix. But here users would expect client: server is not capable
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, fair enough
9ebccd8
to
fba1925
Compare
fba1925
to
51435df
Compare
Codecov Report
@@ Coverage Diff @@
## master #7342 +/- ##
=========================================
Coverage ? 63.67%
=========================================
Files ? 233
Lines ? 21016
Branches ? 0
=========================================
Hits ? 13381
Misses ? 6627
Partials ? 1008
Continue to review full report at Codecov.
|
all fixed ptal |
LGTM. /cc @xiang90 |
LGTM |
Adds a
UseLatestCluster
flag (and user-defined context) to config.Fixes #6579